Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding halogens kinetics (twin with DB) #2135

Merged
merged 5 commits into from
Jun 3, 2023
Merged

Adding halogens kinetics (twin with DB) #2135

merged 5 commits into from
Jun 3, 2023

Conversation

davidfarinajr
Copy link
Contributor

@davidfarinajr davidfarinajr commented May 19, 2021 •
edited
Loading

This PR is a simultaneous with ReactionMechanismGenerator/RMG-database#515

It adds a commit to swap labels for halogen abstraction reactions since these families are their own reverse (similar to h abstraction).
It also swaps labels for 1,2_xy_interchange which is also its own reverse

@davidfarinajr davidfarinajr mentioned this pull request May 19, 2021
Closed
@codecov
Copy link

codecov bot commented May 19, 2021 •
edited
Loading

Codecov Report

Merging #2135 (ace72d4) into main (40b5558) will decrease coverage by 0.01%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #2135      +/-   ##
==========================================
- Coverage   48.15%   48.14%   -0.01%     
==========================================
  Files         110      110              
  Lines       30626    30629       +3     
  Branches     7988     7989       +1     
==========================================
  Hits        14747    14747              
- Misses      14351    14353       +2     
- Partials     1528     1529       +1
Impacted Files Coverage Δ
rmgpy/data/kinetics/family.py 48.05% <40.00%> (-0.06%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@davidfarinajr davidfarinajr force-pushed the halogens_kinetics branch 2 times, most recently from 18cb7ab to e0ec3ce Compare August 4, 2021 19:39
@davidfarinajr davidfarinajr requested review from rwest and mazeau August 4, 2021 19:40
@davidfarinajr davidfarinajr mentioned this pull request Aug 10, 2021
Open
@sevyharris sevyharris self-requested a review September 1, 2021 22:16
@sevyharris
Copy link
Contributor

I tried running the 2-BTP example file and it errored out after about 2 minutes. I'll see if I can figure out why, but here's the error message if you have time to take a look:

Traceback (most recent call last):
  File "/home/moon/rmg/RMG-Py//rmg.py", line 118, in <module>
    main()
  File "/home/moon/rmg/RMG-Py//rmg.py", line 112, in main
    rmg.execute(**kwargs)
  File "/home/moon/rmg/RMG-Py/rmgpy/rmg/main.py", line 925, in execute
    trimolecular_react=self.trimolecular_react)
  File "/home/moon/rmg/RMG-Py/rmgpy/rmg/model.py", line 654, in enlarge
    self.apply_kinetics_to_reaction(reaction)
  File "/home/moon/rmg/RMG-Py/rmgpy/rmg/model.py", line 897, in apply_kinetics_to_reaction
    kinetics, source, entry, is_forward = self.generate_kinetics(reaction)
  File "/home/moon/rmg/RMG-Py/rmgpy/rmg/model.py", line 925, in generate_kinetics
    return_all_kinetics=False)
  File "/home/moon/rmg/RMG-Py/rmgpy/data/kinetics/family.py", line 2711, in get_kinetics
    kinetics, entry = self.get_kinetics_for_template(template, degeneracy, method=estimator)
  File "/home/moon/rmg/RMG-Py/rmgpy/data/kinetics/family.py", line 2628, in get_kinetics_for_template
    return self.estimate_kinetics_using_rate_rules(template, degeneracy)  # This returns kinetics and entry data
  File "/home/moon/rmg/RMG-Py/rmgpy/data/kinetics/family.py", line 2779, in estimate_kinetics_using_rate_rules
    kinetics, entry = self.rules.estimate_kinetics(template, degeneracy)
  File "/home/moon/rmg/RMG-Py/rmgpy/data/kinetics/rules.py", line 622, in estimate_kinetics
    while entry.parent is not None:

The full log of the RMG run is here: RMG.log

@davidfarinajr
Copy link
Contributor Author

Thanks for reviewing this @sevyharris ! Not sure exactly what's going wrong here, but I can take a closer look later this week. I am not sure what version of the database you are using to run the example, but I think we need to use the DB from both ReactionMechanismGenerator/RMG-database#515 and ReactionMechanismGenerator/RMG-database#514 for it to work

@sevyharris
Copy link
Contributor

Okay, I've probably got the wrong version of the database. Is there a single database branch that has the changes from both 514 and 515? I was using halogen_kinetics for RMG-database, but I see there's also halogen_training_reactions and only_halogens_families. Are any of these the correct one to use?

Also for RMG-Py I was using the halogens_kinetics branch.

@davidfarinajr davidfarinajr force-pushed the halogens_kinetics branch 2 times, most recently from 1280113 to c55060d Compare September 15, 2021 19:01
@davidfarinajr davidfarinajr changed the base branch from master to main October 1, 2021 17:40
sevyharris
sevyharris previously approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@sevyharris sevyharris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Also, I got the 2-BTP example to work.

@davidfarinajr @rwest
added halogen abstraction families to species cases in family.py
cc23b71
@davidfarinajr @rwest
swap labels for 1,2_xy_interchange reaction family
af89bfc
@davidfarinajr @rwest
added rmg halogens example of 2btp/CF3Br in CH4 flames
367c17f
@davidfarinajr @rwest
changed path reactions test from 7 to [6,7]
e39b093 
This test was originally 6, then changed to seven when CO was added to library, now it wants to be 7 again.  Since is flucuates between 6 and 7 with small changed in DB, I amn chaning it here to `[6,7]`
@davidfarinajr @rwest
skip kinetics_check_groups_nonidentical for auto generated families
ace72d4 
autogen trees should not have identical groups, so this test is unnecessary
@rwest rwest force-pushed the halogens_kinetics branch from 9fb6db3 to ace72d4 Compare June 2, 2023 19:06
rwest
rwest approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commits and the changes look OK to me. Sevy reviewed it previously and also approved. I have just rebased it, with no conflicts, and have run the example input file again (with an appropriate halogens database, which I'll deal with in a separate PR). So this seems to be ok to me. Let's let the CI tests do their thing.

@rwest rwest enabled auto-merge June 2, 2023 19:10
@rwest rwest merged commit d57bb10 into main Jun 3, 2023
@rwest rwest deleted the halogens_kinetics branch June 3, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwest rwest rwest approved these changes

@sevyharris sevyharris sevyharris left review comments

@mazeau mazeau Awaiting requested review from mazeau

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidfarinajr @sevyharris @rwest